Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)" #109422

sdkrystian · 2024-09-20T13:48:07Z

Reapplies #104458, fixing a bug that occurs when a class member access expression calls an operator-> operator function that returns a non-dependent class type.

llvmbot · 2024-09-20T13:48:43Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Reapplies #104458, fixing a bug that occurs when a templated class declares operator-> to return a non-dependent class type.

Full diff: https://github.com/llvm/llvm-project/pull/109422.diff

6 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+3-3)
(modified) clang/lib/Sema/SemaExprCXX.cpp (+18-18)
(modified) clang/lib/Sema/SemaExprMember.cpp (+4-2)
(modified) clang/lib/Sema/SemaOverload.cpp (+19-5)
(modified) clang/lib/Sema/TreeTransform.h (+6-2)
(modified) clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp (+13-11)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 68c782a15c6f1b..c1afefd36ce0ce 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10639,9 +10639,9 @@ class Sema final : public SemaBase {
   /// BuildOverloadedArrowExpr - Build a call to an overloaded @c operator->
   ///  (if one exists), where @c Base is an expression of class type and
   /// @c Member is the name of the member we're trying to find.
-  ExprResult BuildOverloadedArrowExpr(Scope *S, Expr *Base,
-                                      SourceLocation OpLoc,
-                                      bool *NoArrowOperatorFound = nullptr);
+  ExprResult BuildOverloadedArrowExpr(Expr *Base, SourceLocation OpLoc,
+                                      bool *NoArrowOperatorFound,
+                                      bool &IsDependent);
 
   ExprResult BuildCXXMemberCallExpr(Expr *Exp, NamedDecl *FoundDecl,
                                     CXXConversionDecl *Method,
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 14feafd1e6b17f..7f425b62b3e2cb 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -7966,18 +7966,6 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
 
   QualType BaseType = Base->getType();
   MayBePseudoDestructor = false;
-  if (BaseType->isDependentType()) {
-    // If we have a pointer to a dependent type and are using the -> operator,
-    // the object type is the type that the pointer points to. We might still
-    // have enough information about that type to do something useful.
-    if (OpKind == tok::arrow)
-      if (const PointerType *Ptr = BaseType->getAs<PointerType>())
-        BaseType = Ptr->getPointeeType();
-
-    ObjectType = ParsedType::make(BaseType);
-    MayBePseudoDestructor = true;
-    return Base;
-  }
 
   // C++ [over.match.oper]p8:
   //   [...] When operator->returns, the operator-> is applied  to the value
@@ -7992,7 +7980,8 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
     SmallVector<FunctionDecl*, 8> OperatorArrows;
     CTypes.insert(Context.getCanonicalType(BaseType));
 
-    while (BaseType->isRecordType()) {
+    while (
+        isa<InjectedClassNameType, RecordType>(BaseType.getCanonicalType())) {
       if (OperatorArrows.size() >= getLangOpts().ArrowDepth) {
         Diag(OpLoc, diag::err_operator_arrow_depth_exceeded)
           << StartingType << getLangOpts().ArrowDepth << Base->getSourceRange();
@@ -8002,15 +7991,26 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
         return ExprError();
       }
 
+      bool IsDependent;
       Result = BuildOverloadedArrowExpr(
-          S, Base, OpLoc,
+          Base, OpLoc,
           // When in a template specialization and on the first loop iteration,
           // potentially give the default diagnostic (with the fixit in a
           // separate note) instead of having the error reported back to here
           // and giving a diagnostic with a fixit attached to the error itself.
           (FirstIteration && CurFD && CurFD->isFunctionTemplateSpecialization())
               ? nullptr
-              : &NoArrowOperatorFound);
+              : &NoArrowOperatorFound,
+          IsDependent);
+
+      if (IsDependent) {
+        // BuildOverloadedArrowExpr sets IsDependent to indicate that we need
+        // to build a dependent overloaded arrow expression.
+        assert(Base->isTypeDependent());
+        BaseType = Context.DependentTy;
+        break;
+      }
+
       if (Result.isInvalid()) {
         if (NoArrowOperatorFound) {
           if (FirstIteration) {
@@ -8030,6 +8030,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
         }
         return ExprError();
       }
+
       Base = Result.get();
       if (CXXOperatorCallExpr *OpCall = dyn_cast<CXXOperatorCallExpr>(Base))
         OperatorArrows.push_back(OpCall->getDirectCallee());
@@ -8067,7 +8068,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
   // it's legal for the type to be incomplete if this is a pseudo-destructor
   // call.  We'll do more incomplete-type checks later in the lookup process,
   // so just skip this check for ObjC types.
-  if (!BaseType->isRecordType()) {
+  if (BaseType->isDependentType() || !BaseType->isRecordType()) {
     ObjectType = ParsedType::make(BaseType);
     MayBePseudoDestructor = true;
     return Base;
@@ -8078,8 +8079,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
   //   Unlike the object expression in other contexts, *this is not required to
   //   be of complete type for purposes of class member access (5.2.5) outside
   //   the member function body.
-  if (!BaseType->isDependentType() &&
-      !isThisOutsideMemberFunctionBody(BaseType) &&
+  if (!isThisOutsideMemberFunctionBody(BaseType) &&
       RequireCompleteType(OpLoc, BaseType,
                           diag::err_incomplete_member_access)) {
     return CreateRecoveryExpr(Base->getBeginLoc(), Base->getEndLoc(), {Base});
diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index f1ba26f38520a9..864e58cc7ee6ef 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -580,7 +580,9 @@ Sema::ActOnDependentMemberExpr(Expr *BaseExpr, QualType BaseType,
     }
   }
 
-  assert(BaseType->isDependentType() || NameInfo.getName().isDependentName() ||
+  assert(BaseType->isDependentType() ||
+         (BaseExpr && BaseExpr->isTypeDependent()) ||
+         NameInfo.getName().isDependentName() ||
          isDependentScopeSpecifier(SS) ||
          (TemplateArgs && llvm::any_of(TemplateArgs->arguments(),
                                        [](const TemplateArgumentLoc &Arg) {
@@ -1313,7 +1315,7 @@ static ExprResult LookupMemberExpr(Sema &S, LookupResult &R,
       BaseType = Ptr->getPointeeType();
     else if (BaseType->isFunctionType())
       goto fail;
-    else if (BaseType->isDependentType())
+    else if (BaseExpr.get()->isTypeDependent())
       BaseType = S.Context.DependentTy;
     else if (BaseType->isRecordType()) {
       // Recover from arrow accesses to records, e.g.:
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 861b0a91240b3b..6b91d4898674ec 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -15878,12 +15878,14 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj,
   return CheckForImmediateInvocation(MaybeBindToTemporary(TheCall), Method);
 }
 
-ExprResult
-Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
-                               bool *NoArrowOperatorFound) {
-  assert(Base->getType()->isRecordType() &&
+ExprResult Sema::BuildOverloadedArrowExpr(Expr *Base, SourceLocation OpLoc,
+                                          bool *NoArrowOperatorFound,
+                                          bool &IsDependent) {
+  assert(Base->getType()->getAsRecordDecl() &&
          "left-hand side must have class type");
 
+  IsDependent = false;
+
   if (checkPlaceholderForOverload(*this, Base))
     return ExprError();
 
@@ -15904,7 +15906,19 @@ Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
     return ExprError();
 
   LookupResult R(*this, OpName, OpLoc, LookupOrdinaryName);
-  LookupQualifiedName(R, Base->getType()->castAs<RecordType>()->getDecl());
+  LookupParsedName(R, /*S=*/nullptr, /*SS=*/nullptr, Base->getType());
+
+  // If the expression is dependent and we either:
+  // - found a member of the current instantiation named 'operator->', or
+  // - found nothing, and the lookup context has no dependent base classes
+  //
+  // then we should build a dependent class member access expression.
+  if (R.wasNotFoundInCurrentInstantiation() ||
+      (Base->isTypeDependent() && !R.empty())) {
+    IsDependent = true;
+    return Base;
+  }
+
   R.suppressAccessDiagnostics();
 
   for (LookupResult::iterator Oper = R.begin(), OperEnd = R.end();
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 0daf620b4123e4..235b51a33babd5 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -16583,10 +16583,14 @@ ExprResult TreeTransform<Derived>::RebuildCXXOperatorCallExpr(
   } else if (Op == OO_Arrow) {
     // It is possible that the type refers to a RecoveryExpr created earlier
     // in the tree transformation.
-    if (First->getType()->isDependentType())
+    if (First->containsErrors())
       return ExprError();
+    bool IsDependent;
     // -> is never a builtin operation.
-    return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
+    ExprResult Result = SemaRef.BuildOverloadedArrowExpr(
+        First, OpLoc, /*NoArrowOperatorFound=*/nullptr, IsDependent);
+    assert(!IsDependent);
+    return Result;
   } else if (Second == nullptr || isPostIncDec) {
     if (!First->getType()->isOverloadableType() ||
         (Op == OO_Amp && getSema().isQualifiedMemberAccess(First))) {
diff --git a/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp b/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
index f32f49ef4539a5..89c22a0bc137d9 100644
--- a/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
+++ b/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
@@ -484,16 +484,19 @@ namespace N4 {
   template<typename T>
   struct A {
     void not_instantiated(A a, A<T> b, T c) {
-      a->x;
-      b->x;
+      a->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
+      b->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
       c->x;
     }
 
     void instantiated(A a, A<T> b, T c) {
-      a->x; // expected-error {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
-            // expected-error@-1 {{no member named 'x' in 'N4::A<int>'}}
-      b->x; // expected-error {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
-            // expected-error@-1 {{no member named 'x' in 'N4::A<int>'}}
+      // FIXME: We should only emit a single diagnostic suggesting to use '.'!
+      a->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
+            // expected-error@-1 {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
+            // expected-error@-2 {{no member named 'x' in 'N4::A<int>'}}
+      b->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
+            // expected-error@-1 {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
+            // expected-error@-2 {{no member named 'x' in 'N4::A<int>'}}
       c->x; // expected-error {{member reference type 'int' is not a pointer}}
     }
   };
@@ -540,11 +543,10 @@ namespace N4 {
       a->T::f();
       a->T::g();
 
-      // FIXME: 'U' should be a dependent name, and its lookup context should be 'a.operator->()'!
-      a->U::x; // expected-error {{use of undeclared identifier 'U'}}
-      a->U::y; // expected-error {{use of undeclared identifier 'U'}}
-      a->U::f(); // expected-error {{use of undeclared identifier 'U'}}
-      a->U::g(); // expected-error {{use of undeclared identifier 'U'}}
+      a->U::x;
+      a->U::y;
+      a->U::f();
+      a->U::g();
     }
 
     void instantiated(D a) {

erichkeane

Does this need a release note? Also, can you point out the 'diff' from the previous commit?

I'm really not a fan of the 'out' parameter, I'd prefer perhaps returning a std::pair + structured binding. WDYT?

erichkeane · 2024-09-20T14:19:31Z

clang/include/clang/Sema/Sema.h

-                                      bool *NoArrowOperatorFound = nullptr);
+  ExprResult BuildOverloadedArrowExpr(Expr *Base, SourceLocation OpLoc,
+                                      bool *NoArrowOperatorFound,
+                                      bool &IsDependent);


Ooof, I'm almost always really against 'out' parameters in C++. I'd hope we could come up with a better way for this.

erichkeane · 2024-09-20T14:20:06Z

clang/lib/Sema/SemaExprCXX.cpp

@@ -8002,15 +7991,26 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
        return ExprError();
      }

+      bool IsDependent;


Please initialize this to false anyway.

@sdkrystian ^

erichkeane · 2024-09-20T14:25:42Z

clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp

-            // expected-error@-1 {{no member named 'x' in 'N4::A<int>'}}
-      b->x; // expected-error {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
-            // expected-error@-1 {{no member named 'x' in 'N4::A<int>'}}
+      // FIXME: We should only emit a single diagnostic suggesting to use '.'!


This is unfortunate, it looks like we're having this problem diagnosing in both phases here, and can get worse with partial instantiations. Typically we'd want to solve this by replacing the expression with a RecoveryExpr or something to suppress it in the instantiated version, can you look into something like that?

sdkrystian · 2024-09-20T16:05:07Z

Does this need a release note?

I don't think so, since this fixes a bug introduced in the same release.

Also, can you point out the 'diff' from the previous commit?

The condition of the assert in SemaExprCXX.cpp was changed from BaseType->isDependentType() to Base->isTypeDependent(), as was the condition on line 1318 in SemaExprMember.cpp.

I'm really not a fan of the 'out' parameter, I'd prefer perhaps returning a std::pair + structured binding. WDYT?

My initial solution was to return ExprEmpty(), but I changed to using an out parameter in response to review feedback (I don't like out parameters either).

porglezomp · 2024-09-23T20:08:12Z

~~I'm testing this and seeing some further behavior changes on dependent typenames with ->. I'm currently trying to reduce and determine if the behavior is correct or not.~~

Nevermind, it seems like this is an issue with our clang fork and doesn't impact this change.

porglezomp · 2024-12-05T23:25:12Z

This appears to cause incorrect behavior in the following code:

struct Pointee {
    template<typename T>
    T *method() { return nullptr; }
};

template <typename T>
struct Pointer {
    T *pointer;
    T *operator->() { return pointer; }
};

struct Base {
    Pointer<Pointee> field;
};

template <typename T>
struct Example: public Base {
    bool example() {
        return field->method<T>() == nullptr;
    }
};

The field->method<T>() is diagnosed as being a dependent template name and it doesn't appear to be one.

cor3ntin · 2025-01-19T08:50:43Z

@sdkrystian ping

…verloaded 'operator->' in the current instantiation (llvm#104458)"

cor3ntin

Can we add a release note?
There are still a few comments from @erichkeane that are not addressed.

Thanks!

cor3ntin · 2025-01-21T08:10:16Z

clang/lib/Sema/SemaExprCXX.cpp

@@ -8002,15 +7991,26 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
        return ExprError();
      }

+      bool IsDependent;


@sdkrystian ^

sdkrystian · 2025-01-21T17:59:50Z

This appears to cause incorrect behavior in the following code:

struct Pointee {
    template<typename T>
    T *method() { return nullptr; }
};

template <typename T>
struct Pointer {
    T *pointer;
    T *operator->() { return pointer; }
};

struct Base {
    Pointer<Pointee> field;
};

template <typename T>
struct Example: public Base {
    bool example() {
        return field->method<T>() == nullptr;
    }
};

The field->method<T>() is diagnosed as being a dependent template name and it doesn't appear to be one.

@porglezomp I just pushed a commit that should fix it.

sdkrystian · 2025-01-21T20:53:07Z

@erichkeane I pushed a commit (10ba129) which eliminates the out parameter in favor of building a CXXOperatorCallExpr with type ASTContext::DependentTy. WDYT?

cor3ntin

LGTM

sdkrystian · 2025-01-22T18:43:33Z

Reverting due to a failure building clang.

…ilding overloaded 'operator->' in the current instantiation (#104458)"" (#123982) Reverts #109422

…ext when building overloaded 'operator->' in the current instantiation (#104458)"" (#123982) Reverts llvm/llvm-project#109422

sdkrystian requested a review from Endilll as a code owner September 20, 2024 13:48

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 20, 2024

sdkrystian requested review from cor3ntin and nikic September 20, 2024 14:09

cor3ntin requested review from AaronBallman and erichkeane September 20, 2024 14:14

erichkeane reviewed Sep 20, 2024

View reviewed changes

porglezomp mentioned this pull request Sep 23, 2024

clang fails to find operator -> #104268

Open

sdkrystian added 8 commits January 20, 2025 12:03

Reapply "[Clang][Sema] Use the correct lookup context when building o…

6edb145

…verloaded 'operator->' in the current instantiation (llvm#104458)"

[FOLD] fix destructor lookup when the object expression is dependent

a4ad2b9

[FOLD] use IsDependent output parameter

0380314

[FOLD] don't use getAsRecordDecl

772a6c0

[FOLD] fix capitalization

51a3384

[FOLD] fix dependent overloaded arrow

e3326ab

[FOLD] cleanup

764a9a8

[FOLD] add test

30297f7

sdkrystian force-pushed the reapply-fix-overloaded-arrow branch from 0d4beec to 30297f7 Compare January 20, 2025 17:04

cor3ntin reviewed Jan 21, 2025

View reviewed changes

[FOLD] fix dependence check

dead49d

[FOLD] address review feedback

10ba129

erichkeane approved these changes Jan 21, 2025

View reviewed changes

[FOLD] revert assert condition

039fbbc

cor3ntin approved these changes Jan 21, 2025

View reviewed changes

[FOLD] cleanup

7d0ac98

sdkrystian merged commit 195a1fc into llvm:main Jan 22, 2025
8 checks passed

sdkrystian mentioned this pull request Jan 22, 2025

Revert "Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)"" #123982

Merged

sdkrystian added a commit that referenced this pull request Jan 22, 2025

Revert "Reapply "[Clang][Sema] Use the correct lookup context when bu…

5ede7b6

…ilding overloaded 'operator->' in the current instantiation (#104458)"" (#123982) Reverts #109422

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)" #109422

Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)" #109422

sdkrystian commented Sep 20, 2024 •

edited

Loading

llvmbot commented Sep 20, 2024

erichkeane left a comment

erichkeane Sep 20, 2024

erichkeane Sep 20, 2024

cor3ntin Jan 21, 2025

erichkeane Sep 20, 2024

sdkrystian commented Sep 20, 2024

porglezomp commented Sep 23, 2024 •

edited

Loading

porglezomp commented Dec 5, 2024

cor3ntin commented Jan 19, 2025

cor3ntin left a comment

cor3ntin Jan 21, 2025

sdkrystian commented Jan 21, 2025

sdkrystian commented Jan 21, 2025

cor3ntin left a comment

sdkrystian commented Jan 22, 2025

Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)" #109422

Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)" #109422

Conversation

sdkrystian commented Sep 20, 2024 • edited Loading

llvmbot commented Sep 20, 2024

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Sep 20, 2024

Choose a reason for hiding this comment

erichkeane Sep 20, 2024

Choose a reason for hiding this comment

cor3ntin Jan 21, 2025

Choose a reason for hiding this comment

erichkeane Sep 20, 2024

Choose a reason for hiding this comment

sdkrystian commented Sep 20, 2024

porglezomp commented Sep 23, 2024 • edited Loading

porglezomp commented Dec 5, 2024

cor3ntin commented Jan 19, 2025

cor3ntin left a comment

Choose a reason for hiding this comment

cor3ntin Jan 21, 2025

Choose a reason for hiding this comment

sdkrystian commented Jan 21, 2025

sdkrystian commented Jan 21, 2025

cor3ntin left a comment

Choose a reason for hiding this comment

sdkrystian commented Jan 22, 2025

sdkrystian commented Sep 20, 2024 •

edited

Loading

porglezomp commented Sep 23, 2024 •

edited

Loading